Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[jrubyscripting] JRuby scripting initial contribution #11538

Merged
merged 17 commits into from
Nov 15, 2021

Conversation

boc-tothefuture
Copy link
Contributor

[jruby] JRuby Scripting Binding initial contribution

This is the initial contribution of the JRuby Scripting Binding based on JRuby 9.3.1.0

This is essentially the same as the Jython binding, with the following differences (which are also noted in the readme)

  1. The File variable, referencing java.io.File is not available as it conflicts with Ruby's File class preventing Ruby from initializing
  2. scriptExtension, automationManager, ruleRegistry, items, voice, rules, things, events, itemRegistry, ir, actions, se, audio, lifecycleTracker are prepended with a $ (e.g. $automationManager) making them available as a global objects in Ruby.
  3. This binding has the (optional) ability to install Ruby Gems if configured - enabling users easily take advantage of 3rd party libraries within their rules.

@boc-tothefuture boc-tothefuture requested a review from a team as a code owner November 7, 2021 04:14
@boc-tothefuture boc-tothefuture changed the title feat(jruby) JRuby scripting initial binding commit feat(jruby): JRuby scripting initial binding commit Nov 7, 2021
@wborn wborn changed the title feat(jruby): JRuby scripting initial binding commit [jrubyscripting] JRuby scripting initial contribution Nov 7, 2021
wborn
wborn previously requested changes Nov 7, 2021
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I had a quick look and noticed these artifact ID mismatches. Maybe it also helps with fixing the build. 🙂

bom/openhab-addons/pom.xml Outdated Show resolved Hide resolved
CODEOWNERS Outdated Show resolved Hide resolved
@boc-tothefuture boc-tothefuture force-pushed the jruby branch 2 times, most recently from 93ad7d6 to 0acab67 Compare November 7, 2021 21:54
@boc-tothefuture boc-tothefuture requested a review from wborn November 7, 2021 21:54
@boc-tothefuture boc-tothefuture force-pushed the jruby branch 2 times, most recently from a4e3871 to 4f73030 Compare November 8, 2021 04:18
@wborn wborn removed their request for review November 8, 2021 07:22
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few more comments. 🙂

Can you also fix the remaining static analysis warnings (SAT)?

CODEOWNERS Outdated Show resolved Hide resolved
bom/openhab-addons/pom.xml Outdated Show resolved Hide resolved
bundles/org.openhab.automation.jrubyscripting/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.automation.jrubyscripting/pom.xml Outdated Show resolved Hide resolved
@boc-tothefuture boc-tothefuture force-pushed the jruby branch 3 times, most recently from a4a6ce0 to ffc9556 Compare November 8, 2021 14:21
@boc-tothefuture boc-tothefuture requested a review from wborn November 8, 2021 14:26
Copy link
Contributor

@jimtng jimtng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the package name (and the corresponding directory) should be org.openhab.automation.jrubyscripting

@wborn
Copy link
Member

wborn commented Nov 8, 2021

Can you stop squashing and force pushing your commits @boc-tothefuture?
It makes it unneccessary difficult to review changes.

@boc-tothefuture
Copy link
Contributor Author

Sorry - I am used to projects that want a single commit to be merged. No problem.

@wborn
Copy link
Member

wborn commented Nov 8, 2021

Great! We always squash commits when merging PRs by default after reviewing. Also don't worry about the DCO check, we can also set it to pass manually if somehow not all commits were properly signed off. 🙂

Copy link
Contributor

@jimtng jimtng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The directory in which the java files are located, needs to change from:
org.openhab.automation.jrubyscripting/src/main/java/org/openhab/binding/jrubyscripting/internal
to
org.openhab.automation.jrubyscripting/src/main/java/org/openhab/automation/jrubyscripting/internal

boc-tothefuture and others added 2 commits November 8, 2021 19:32
…g/openhab/binding/jrubyscripting/internal/JRubyScriptEngineFactory.java

Co-authored-by: Wouter Born <[email protected]>
boc-tothefuture and others added 6 commits November 8, 2021 19:33
…g/openhab/binding/jrubyscripting/internal/JRubyScriptEngineConfiguration.java

Co-authored-by: Wouter Born <[email protected]>
…g/openhab/binding/jrubyscripting/internal/JRubyScriptEngineFactory.java

Co-authored-by: Wouter Born <[email protected]>
…g/openhab/binding/jrubyscripting/internal/JRubyScriptEngineFactory.java

Co-authored-by: Wouter Born <[email protected]>
[jrubyscripting] Add ConfigurableService
@boc-tothefuture
Copy link
Contributor Author

@wborn I think the latest commit from @jimtng address all outstanding comments.

Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my comments!
I still have these minor comments left now. 🙂

@boc-tothefuture
Copy link
Contributor Author

boc-tothefuture commented Nov 15, 2021

@wborn can you take another look?

@jimtng pushed some change sets that I think address your remaining concerns.

From @jimtng PR Comments:

The remaining SAT warnings appear to be false positives.

I had to revert the configuration format in the docs because it didn't work without the prefixes. It appeared to work on my system because openhab auto-created the config file in userdata/config/org/openhab/automation/jrubyscripting.config and kept it there even after the correct entries (with the prefix) in jruby.cfg were removed.

Thanks @jimtng for pushing this over the finish line.

Thanks @wborn for the thorough review and guidance.

Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks @boc-tothefuture and @jimtng!
It's nice to also have JRuby Scripting ♦️ as an official automation add-on. 👍

@wborn wborn merged commit 6335825 into openhab:main Nov 15, 2021
@wborn wborn added this to the 3.2 milestone Nov 15, 2021
@wborn
Copy link
Member

wborn commented Nov 15, 2021

Now that the add-on is merged, it would also be nice if the add-on has an icon in the docs and when it is featured as automation add-on on the website. 🙂

That can be done with a couple of PRs similar like:

@jimtng
Copy link
Contributor

jimtng commented Nov 16, 2021

openhab/website#305
@wborn would you mind adding the logo please? I'm not familiar with the legal aspects of it and I'd rather not get involved with such matter. The logo can be found at https://www.jruby.org/images/jruby-logo.png

@wborn
Copy link
Member

wborn commented Nov 16, 2021

Maybe @ghys knows a bit more about the legal aspects because he also created #8513. AFAIK the guidelines for using the openHAB logo itself are also still undefined.

It might also help to just automatically generate logos based on add-on names. That way the add-on overview looks nicer and it is easier to identify add-ons in the UI. For instance you can make text look cooler using: https://maketext.io/ 😎

@ghys
Copy link
Member

ghys commented Nov 17, 2021

In https://github.com/openhab/openhab-docs/tree/main/images/addons there is a LICENSE file that explicitly excludes the contents of that directory from being covered by the EPL like the rest of the repo.

Note: These images are NOT made available under the repo license (EPL).

All product names, trademarks and registered trademarks are property of their respective owners.
All company, product and service names used are for identification purposes only. 
Use of these names, trademarks and brands does not imply endorsement.

We invoke fair use and make some disclaimers, with this I think we're in the clear - especially since we only use these logos to build the website and the new add-on store UI merely links to them. This can be considered "editorial content" (https://www.upcounsel.com/permission-to-use-logo#:~:text=Fair%20use%20includes%20using%20a,or%20associates%20with%20another%20company).

NickWaterton pushed a commit to NickWaterton/openhab-addons that referenced this pull request Dec 30, 2021
Also-by: Jimmy Tanagra <[email protected]>
Signed-off-by: Brian O'Connell <[email protected]>
Signed-off-by: Nick Waterton <[email protected]>
mischmidt83 pushed a commit to mischmidt83/openhab-addons that referenced this pull request Jan 9, 2022
Also-by: Jimmy Tanagra <[email protected]>
Signed-off-by: Brian O'Connell <[email protected]>
Signed-off-by: Michael Schmidt <[email protected]>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Jan 28, 2022
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants